PP-337 Quicksight dashboard embed URL generation #1378

RishiDiwanTT · 2023-09-13T11:21:28Z

Description

Depends on a comma separated string of dashboard ARNs under QUICKSIGHT_AUTHORIZED_ARNS environment variable.
Exposes the URL /admin/quicksight_embed/<dashboard-id>?libraryIds=1,2,3 and returns dict(embedUrl=....).

Currently allows access till the LIBRARIAN admin role.

Motivation and Context

Certain library Admin users need on-the-go and anonymous access to the quicksight dashboards for analytics.
JIRA

How Has This Been Tested?

Manually tested the link generation with the tpp-dev account.
New unit tests were written.

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2023-09-13T11:28:15Z

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (5484028) 90.29% compared to head (4b562bd) 90.31%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1378      +/-   ##
==========================================
+ Coverage   90.29%   90.31%   +0.01%     
==========================================
  Files         232      234       +2     
  Lines       29751    29841      +90     
  Branches     6797     6820      +23     
==========================================
+ Hits        26864    26950      +86     
- Misses       1843     1847       +4     
  Partials     1044     1044

Files	Coverage Δ
api/admin/controller/__init__.py	`100.00% <100.00%> (ø)`
api/admin/model/quicksight.py	`100.00% <100.00%> (ø)`
core/util/flask_util.py	`93.10% <100.00%> (+0.51%)`	⬆️
api/admin/routes.py	`95.13% <88.88%> (+0.55%)`	⬆️
api/admin/controller/quicksight.py	`90.74% <90.74%> (ø)`

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

RishiDiwanTT · 2023-09-15T06:07:21Z

api/admin/routes.py

-        v = f(*args, **kwargs)
+        try:
+            v = f(*args, **kwargs)
+        except ProblemError as ex:


Added this in so we can globally manage raising ProblemErrors

jonathangreen · 2023-09-22T19:03:16Z

This one looks okay to me, but I'd like @dbernstein to take a look since hes done the rest of the quicksight work.

dbernstein · 2023-09-26T18:29:51Z

README.md

+##### Quicksight Dashboards
+
+For generating quicksight dashboard links the following environment variable is required
+`QUICKSIGHT_AUTHORIZED_ARNS` - A comma separated list of `arn:aws:quicksight:...` format ARN strings


Since we'll likely have multiple dashboards that can be displayed within the CM, I think we'll need to pass in a dictionary where the keys are logical dashboard names such as "library" or "consortium" or "all_libraries" and the values are ARNs that link to environment specifi dashboards. To decouple the logical name from the dashboard id, I suggest we use the logical name (ie the key) in the Circulation manager API calls below. Those logical names will then be hardcoded in the Admin UI. Does that make sense?

I changed this to a Dict format like:

{ "name": [<arn list>]}

This should be json dumped from ansible into the same env var.
I also added an /admin/quicksight_embed/names API so the dashboard names are not hard coded anywhere (frontend or backend).
This brings up an issue with authorisation though, all dashboard names will be visible to all dashboard users regardless of whether there is any intersecting library between their authorisation and the dashboards available libraries.
In which case the admin API will throw authorisation errors when trying to create an URL without any libraries visible to the user.
While not a security concern, it just becomes a game of guessing which dashboards are available for the user themselves.

dbernstein · 2023-09-26T18:31:51Z

core/config.py

@@ -50,6 +50,10 @@ class Configuration(ConfigurationConstants):
    OD_FULFILLMENT_CLIENT_KEY_SUFFIX = "OVERDRIVE_FULFILLMENT_CLIENT_KEY"
    OD_FULFILLMENT_CLIENT_SECRET_SUFFIX = "OVERDRIVE_FULFILLMENT_CLIENT_SECRET"

+    # Quicksight
+    # Comma separated aws arns
+    QUICKSIGHT_AUTHORIZED_ARNS_KEY = "QUICKSIGHT_AUTHORIZED_ARNS"


See comment above re dictionary.

dbernstein

As usual, excellent work @RishiDiwanTT . See my suggestion re use of a dictionary. I'm curious what your thoughts are on that.

dbernstein · 2023-09-26T18:36:10Z

api/admin/routes.py

@@ -340,6 +348,18 @@ def stats():
    return statistics_response.api_dict()


+@app.route("/admin/quicksight_embed/<dashboard_id>")


I believe we'll want to use the logical name here instead of the dashboard id. That way the Admin UI can use hard-coded logical names which can be completely decoupled from the dashboard ARN. (Per my comments above on the README).

dbernstein

Thanks @RishiDiwanTT : 🚀

Depends on a comma separated string of dashboard ARNs under QUICKSIGHT_AUTHORIZED_ARNS environment variable

…nerics Re-implemented as a functional_validator

Added the a /names API to get the dashboard names available

RishiDiwanTT requested a review from a team September 13, 2023 11:21

RishiDiwanTT self-assigned this Sep 13, 2023

RishiDiwanTT commented Sep 15, 2023

View reviewed changes

jonathangreen requested a review from dbernstein September 22, 2023 19:02

dbernstein reviewed Sep 26, 2023

View reviewed changes

dbernstein requested changes Sep 26, 2023

View reviewed changes

RishiDiwanTT requested a review from dbernstein October 2, 2023 05:28

dbernstein approved these changes Oct 4, 2023

View reviewed changes

RishiDiwanTT added 4 commits October 6, 2023 13:52

Quicksight dashboard embed URL generation

5104cfe

Depends on a comma separated string of dashboard ARNs under QUICKSIGHT_AUTHORIZED_ARNS environment variable

Pydantic 1.1 under python 3.8 does not allow custom datatypes with Ge…

cdc595f

…nerics Re-implemented as a functional_validator

Added test cases for coverage

397055e

Switched from using arrays to a descriptive dict for quicksight ARNs

4b562bd

Added the a /names API to get the dashboard names available

RishiDiwanTT force-pushed the feature/quicksight-embed-url branch from 8371069 to 4b562bd Compare October 6, 2023 08:36

RishiDiwanTT merged commit 714dbd5 into main Oct 6, 2023
30 checks passed

RishiDiwanTT deleted the feature/quicksight-embed-url branch October 6, 2023 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PP-337 Quicksight dashboard embed URL generation #1378

PP-337 Quicksight dashboard embed URL generation #1378

RishiDiwanTT commented Sep 13, 2023

codecov bot commented Sep 13, 2023 •

edited

Loading

RishiDiwanTT Sep 15, 2023

jonathangreen commented Sep 22, 2023

dbernstein Sep 26, 2023 •

edited

Loading

RishiDiwanTT Sep 28, 2023 •

edited

Loading

dbernstein Sep 26, 2023

dbernstein left a comment

dbernstein Sep 26, 2023

dbernstein left a comment

		@@ -340,6 +348,18 @@ def stats():
		return statistics_response.api_dict()


		@app.route("/admin/quicksight_embed/<dashboard_id>")

PP-337 Quicksight dashboard embed URL generation #1378

PP-337 Quicksight dashboard embed URL generation #1378

Conversation

RishiDiwanTT commented Sep 13, 2023

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

RishiDiwanTT Sep 15, 2023

Choose a reason for hiding this comment

jonathangreen commented Sep 22, 2023

dbernstein Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

RishiDiwanTT Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

dbernstein Sep 26, 2023

Choose a reason for hiding this comment

dbernstein left a comment

Choose a reason for hiding this comment

dbernstein Sep 26, 2023

Choose a reason for hiding this comment

dbernstein left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 13, 2023 •

edited

Loading

dbernstein Sep 26, 2023 •

edited

Loading

RishiDiwanTT Sep 28, 2023 •

edited

Loading